Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: Update config.go comments #1636

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Docs: Update config.go comments #1636

wants to merge 6 commits into from

Conversation

aimeeu
Copy link
Contributor

@aimeeu aimeeu commented Mar 29, 2024

Add content to comments that belongs with the field rather than separate content on the docs page.

/kind documentation

Part of ENG-3147

@aimeeu aimeeu marked this pull request as draft March 29, 2024 12:29
@aimeeu aimeeu changed the title Docs: Update config yaml comments Docs: Update config yaml comments WIP Mar 29, 2024
Copy link

netlify bot commented Mar 29, 2024

Deploy Preview for vcluster-docs canceled.

Name Link
🔨 Latest commit d57bd3f
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/660efbc7b7a4130008c9583c

@aimeeu aimeeu changed the title Docs: Update config yaml comments WIP Docs: Update config.go comments Apr 4, 2024
@aimeeu aimeeu marked this pull request as ready for review April 4, 2024 19:13
@aimeeu aimeeu requested a review from FabianKramm April 4, 2024 19:15
@@ -678,7 +678,7 @@ type EtcdDeploy struct {
// Enabled defines that an external etcd should be deployed.
Enabled bool `json:"enabled,omitempty"`

// StatefulSet holds options for the external etcd statefulSet.
// Options for the external etcd StatefulSet. See https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/stateful-set-v1/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: go comments prefer to start the comment with the name of the exported field/type/func. Or at least mention the name in the first sentence. See: https://go.dev/doc/comment#type

This already applies to most of the fields here. So it might be good to stick with this keeping it consistent, WDYT?

Deployment CoreDNSDeployment `json:"deployment,omitempty"`

// OverwriteConfig can be used to overwrite the coredns config
// Overwrite default config. Path to a custom Corefile. See https://coredns.io/2017/07/23/corefile-explained/.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -870,7 +871,7 @@ type ControlPlaneAdvanced struct {
// upload all required vCluster images to a single private repository and set this value. Workload images are not affected by this.
DefaultImageRegistry string `json:"defaultImageRegistry,omitempty"`

// VirtualScheduler defines if a scheduler should be used within the virtual cluster or the scheduling decision for workloads will be made by the host cluster.
// Defines if a scheduler should be used within the virtual cluster or the scheduling decision for workloads will be made by the host cluster.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@aimeeu
Copy link
Contributor Author

aimeeu commented Apr 5, 2024

Thanks for your comments @johannesfrey
For the docs, starting the comment with the name of the exported field/type/func is redundant.

For example:
image

I can work within Go comment style guidelines if if I have to. For user-facing text, this is a case of deciding whether to strictly follow Go comments style or present content that's more user-friendly but doesn't strictly follow Go comment style rules. We aren't generating a Go API guide or Go library docs, if you see my point.

Probably better to let this PR sit until engineering is done updating the YAML structure. Then we can decide how to structure the comments, and I can update in a single attempt (I don't like merging/rebasing lol).

@aimeeu aimeeu marked this pull request as draft April 5, 2024 14:35
@johannesfrey
Copy link
Contributor

Thanks for your comments @johannesfrey For the docs, starting the comment with the name of the exported field/type/func is redundant.

For example: image

I can work within Go comment style guidelines if if I have to. For user-facing text, this is a case of deciding whether to strictly follow Go comments style or present content that's more user-friendly but doesn't strictly follow Go comment style rules. We aren't generating a Go API guide or Go library docs, if you see my point.

Probably better to let this PR sit until engineering is done updating the YAML structure. Then we can decide how to structure the comments, and I can update in a single attempt (I don't like merging/rebasing lol).

Your welcome 🙂 .

Yes I totally get your point. Indeed we should rather strive for user-friendliness instead of strictly following the go style guidelines. Especially as we are using them verbatim as user-facing docs, which I forgot while doing the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants